Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow running tests via CMake build script #1479

Merged
merged 8 commits into from Jul 22, 2020
Merged

Conversation

Martchus
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jul 20, 2020

Codecov Report

Merging #1479 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1479      +/-   ##
==========================================
- Coverage   56.19%   56.18%   -0.01%     
==========================================
  Files          54       54              
  Lines        6357     6356       -1     
==========================================
- Hits         3572     3571       -1     
  Misses       2785     2785              
Impacted Files Coverage Δ
tools/update-deps 93.63% <100.00%> (-0.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc25ddd...e6593f2. Read the comment docs.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good direction. I think separate script files for the checks like yamllint and perl style are a good idea but I am worried about the additional complexity of tools/invoke-tests. In principle we should just be able to call prove -l -r without anything additional and that should work to run all the tests. I don't even know if there is any reason why we still need to explicitly list all the tests within os-autoinst. I know that originally that was necessary but over the years we have actually fixed a lot of problems so maybe we can just go with an approach similar to what we do in openQA and not need an explicit list of test modules anymore. In the spec file we can then simply delete the test files directly instead of only deleting the reference in a test list.

EDIT: I just tried out PERL5LIB=$PWD:$PERL5LIB prove -l -r and that seems to do what it should for all test modules

cmake/test-targets.cmake Outdated Show resolved Hide resolved
tools/invoke-tests Outdated Show resolved Hide resolved
tools/invoke-tests Outdated Show resolved Hide resolved
@Martchus Martchus marked this pull request as ready for review July 20, 2020 15:03
@okurz
Copy link
Member

okurz commented Jul 20, 2020

See #1480 for my proposal to get rid of the hardcoded test list first. After that we could take a look what would still be needed from tools/invoke-tests and try if we can get rid of that script again.

@mergify
Copy link
Contributor

mergify bot commented Jul 20, 2020

This pull request is now in conflicts. Could you fix it? 🙏

…e-tests

* Use getopt to parse arguments
* Don't rely on the script being executed within the source directory
* Improve targets for coverage computation so it is ensured that everything
  is built correctly
* Add target to reset gathered coverage data
* Enable verbose CTest output by default (see comment for reasoning)
@Martchus Martchus force-pushed the cmake branch 2 times, most recently from c668793 to 49d450a Compare July 21, 2020 14:20
tools/invoke-tests Outdated Show resolved Hide resolved
@mergify mergify bot merged commit 3d1c89c into os-autoinst:master Jul 22, 2020
@Martchus Martchus deleted the cmake branch July 22, 2020 10:00
@Martchus
Copy link
Contributor Author

(I've restarted the Travis job after it succeeded but apparently could not report that.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants